Skip to content

Conversation

@mmillns2
Copy link
Member

CARP is now multi-threaded. The PyQt GUI and the controller class (which interfaces with the GUI) sit on the main thread. Now, the AcquisitionWorker class is instantiated on its own thread. Let's call this thread the worker thread. The main thread and worker thread communicate with one another via two thread safe buffers. One buffer is for commands being sent from the GUI on the main thread to the digitiser on the worker thread. The other buffer is for data sent from the worker thread to the main thread that is to be displayed on the GUI. The data structure used for these buffers is a python Queue as it is thread safe by default.

@jwaiton jwaiton self-requested a review November 10, 2025 16:26
@jwaiton
Copy link
Member

jwaiton commented Nov 11, 2025

Some notes:

  • Add documentation explaining what the function does, the input parameters and the expected output as well as type checking. An example can be seen here.
  • Lets move the Command() and CommandType() classes into a separate file, perhaps core/commands.py. Up to you 😸
  • Following the best practices of our favourite object oriented programming language, lets separate out the classes into separate files in core/, so AcquisitionWorker.py and Tracker.py. They can be imported back in very simply, I believe there are examples within CARP to do this already.
  • This is a bit picky of me, but I think it would be nice for comments to generally outline whats happening in the code a bit more discretely, an example would be here:
    def run(self):
        logging.info("AcquisitionWorker thread started.")
        try:
            while not self.stop_event.is_set():
                # Handle commands
                while True:

while not self.stop_event.is_set() is a significant while condition, defining that self.stop_event controls whether or not CARP runs. Something like:

    def run(self):
        logging.info("AcquisitionWorker thread started.")
        try:
            # process commands and data while stop_event isn't set
            while not self.stop_event.is_set():
                # Handle commands
                while True:

Again, this is a bit pedantic of me, but it allows for the program flow to be a bit more natural imo. You can ignore this if you like, I may add them in in the future to understand where and what is happening throughout the code :)

I'll get to actually reviewing the code as soon as I can 😅

Copy link
Member

@jwaiton jwaiton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely bit of code, improving CARP immensely! Very easy to read as well, other than a few minor caveats. I'm happy for this to be approved after the caveats are addressed.

My prior comment on documentation can wait I think (in part because its hypocritical, I'd forgotten how badly I documented the functions myself 🤦‍♂️), I'll add general documentation of functions as an issue and add it into the technical debt (#26). Thanks again for writing this. 😸

@jwaiton
Copy link
Member

jwaiton commented Nov 20, 2025

Looks good! In lieu of actual tests I think it would be nice that we double check that the code works in the lab (we can do this together or I can do it, whatevers easier), then I'll approve the PR and we can get to rebasing and merging.

Copy link
Member

@jwaiton jwaiton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces multi-threading into CARP to deal with the high throughput needed for acquisition purposes. The docstrings and documentation is being left for a later date.

Great job! Lets rebase it and get it merged 😸

All digitiser function calls have been replaced with pushing commands to
a thread safe command buffer.
Added local flags to keep track of digitiser status. Replaced digitiser
member function calls with controller member function calls. Controller
member functions interface with digitiser via a thread-safe buffer.
@jwaiton jwaiton merged commit 556bf8c into nu-ZOO:main Nov 20, 2025
2 of 3 checks passed
@jwaiton jwaiton mentioned this pull request Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants